Skip to content

feat: implement clusterrole primitive#37

Merged
sourcehawk merged 28 commits intomainfrom
feature/clusterrole-primitive
Mar 25, 2026
Merged

feat: implement clusterrole primitive#37
sourcehawk merged 28 commits intomainfrom
feature/clusterrole-primitive

Conversation

@sourcehawk
Copy link
Owner

@sourcehawk sourcehawk commented Mar 22, 2026

Implements the clusterrole Kubernetes resource primitive following the pattern established by the existing ConfigMap and Deployment primitives.

Summary

  • Adds clusterrole primitive package under pkg/primitives/clusterrole/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, flavors, and builder
  • Adds PolicyRulesEditor to pkg/mutation/editors/ (reusable for Role and ClusterRole)
  • Adds cluster-scoped resource support to internal/generic/builder_base.go

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md
  • Shared infrastructure changes are limited to adding cluster-scope support (internal/generic/builder_base.go) and a new PolicyRulesEditor (pkg/mutation/editors/)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new clusterrole primitive to the operator-component-framework so controllers can manage Kubernetes rbac.authorization.k8s.io/v1 ClusterRole objects using the same builder/mutator/flavor patterns as existing primitives.

Changes:

  • Introduces pkg/primitives/clusterrole with Resource, Builder, Mutator, and field-application flavors for ClusterRole-specific behavior.
  • Adds a new shared mutation editor PolicyRulesEditor under pkg/mutation/editors to support typed .rules mutation.
  • Adds an end-to-end example (examples/clusterrole-primitive) and full primitive documentation (docs/primitives/clusterrole.md).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/primitives/clusterrole/resource.go Defines the ClusterRole Resource wrapper and default field applicator.
pkg/primitives/clusterrole/mutator.go Implements plan-and-apply mutator for metadata, rules, and aggregationRule.
pkg/primitives/clusterrole/builder.go Provides fluent builder API and validation/workarounds for cluster-scoped objects.
pkg/primitives/clusterrole/flavors.go Adds flavors to preserve labels/annotations and externally-added rules.
pkg/primitives/clusterrole/*_test.go Adds unit tests for builder, mutator, and flavors.
pkg/mutation/editors/policyrules.go Adds a reusable PolicyRulesEditor for Role/ClusterRole rules mutation.
pkg/mutation/editors/policyrules_test.go Adds tests for PolicyRulesEditor.
examples/clusterrole-primitive/* Adds a runnable example demonstrating mutation composition and reconciliation.
docs/primitives/clusterrole.md Adds user-facing documentation for the new primitive API and usage patterns.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Updated PR description checklist to accurately reflect that shared infrastructure files are modified (internal/generic/builder_base.go for cluster-scope support, pkg/mutation/editors/policyrules.go as a new reusable editor). The old checklist item "Does not modify shared files" has been replaced with a specific description of the shared infrastructure changes.

Intentionally ignored:
None

<!-- claude-review-cycle -->

@sourcehawk
Copy link
Owner Author

approved but wait until last

Copilot AI review requested due to automatic review settings March 23, 2026 00:18
@sourcehawk sourcehawk force-pushed the feature/clusterrole-primitive branch from 102978d to a1d3fca Compare March 23, 2026 00:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Removed duplicate "valid clusterrole without namespace" test case in builder_test.go (identical to "valid clusterrole")
  • Documented cluster-scoped ownership limitation in docs/primitives/clusterrole.md (namespaced owner cannot own a ClusterRole)
  • Added comment in examples/clusterrole-primitive/main.go explaining the namespaced owner limitation with real clusters

Intentionally ignored:

  • resource.go line 28: component.DataExtractable is the consistent convention used across all primitives (deployment, configmap, clusterrole). Changing to concepts.DataExtractable would break codebase consistency.
  • builder_base.go line 96: Valid architectural concern about SetControllerReference failing for cluster-scoped dependents with namespaced owners, but this requires a framework-level change (e.g., adding SkipOwnerReference to ResourceOptions and threading it through the reconciliation path). Documented the limitation instead; framework fix should be tracked as a separate issue.

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 03:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • docs/primitives/clusterrole.md line 7: Updated ownership limitation note to reflect actual framework behavior (scope mismatch is detected, owner ref is skipped with a log message, reconciliation proceeds without GC ownership)
  • docs/primitives/clusterrole.md line 319: Removed PreserveExternalRules from the feature-gating walkthrough example and added a documentation note explaining the trade-off between PreserveExternalRules and feature-gated mutations
  • examples/clusterrole-primitive/resources/clusterrole.go line 40: Removed PreserveExternalRules from the example to avoid conflict with feature-gated mutation add/remove behavior
  • examples/clusterrole-primitive/main.go line 40: Updated comments to describe the actual scope-mismatch behavior (skip with log) rather than implying reconciliation is rejected
  • examples/clusterrole-primitive/README.md line 10: Removed the PreserveExternalRules bullet from the feature list to match the updated example code
  • pkg/primitives/clusterrole/mutator.go line 30: Adopted the per-feature planning pattern (beginFeature/featurePlan) consistent with ConfigMap and Deployment primitives, so mutation ordering respects registration order across features
  • internal/generic/builder_static_test.go line 113: Removed duplicate cluster-scoped validation subtests (kept the testify require/assert versions)

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 16:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings March 23, 2026 16:59
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • PreserveExternalRules slice-order equality (flavors.go:60): Replaced strict slice-order comparison with set-based (sort-then-compare) equality in policyRuleEqual, so reordered-but-equivalent rules from admission webhooks are correctly recognized as duplicates. Added test covering this case.
  • SetAggregationRule pointer aliasing (mutator.go:100): Added DeepCopy of the *AggregationRule at registration time so the mutator records intent immutably, consistent with AddRule which records by value. Added test verifying caller mutation after registration does not affect the applied result.
  • Build() duplicate validation (builder.go:106): Removed local nil-object and empty-name checks, delegating entirely to generic.StaticBuilder.Build()ValidateBase(), consistent with the ConfigMap primitive. Removed unused errors import.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

@sourcehawk sourcehawk force-pushed the feature/clusterrole-primitive branch from 84f470c to 4b6eaa0 Compare March 23, 2026 21:33
Copilot AI review requested due to automatic review settings March 24, 2026 00:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • mutator.go: Added requireActive() guard that panics with a clear message ("BeginFeature must be called before registering mutations") when EditObjectMetadata, EditRules, or SetAggregationRule are called without a prior BeginFeature() call. Added corresponding test TestMutator_PanicsWithoutBeginFeature.

Intentionally ignored:
None

<!-- claude-review-cycle -->

sourcehawk and others added 28 commits March 25, 2026 17:57
Introduces a typed editor for mutating the .rules field of Role and
ClusterRole resources, following the same pattern as existing editors
(ConfigMapDataEditor, ObjectMetaEditor). Provides AddRule,
RemoveRuleByIndex, Clear, and Raw escape hatch operations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the ClusterRole primitive as a Static, cluster-scoped resource.
Includes builder (with name-only validation, no namespace required),
resource, mutator (EditObjectMetadata, EditRules, AddRule,
SetAggregationRule), and flavors (PreserveCurrentLabels,
PreserveCurrentAnnotations, PreserveExternalRules).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the ClusterRole primitive API, mutation system, editors,
flavors, aggregation rule support, and usage patterns.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates feature-gated RBAC rule composition using the clusterrole
primitive, including core rules, version labels, and conditional
secret/deployment access mutations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove feature-plan abstraction from Mutator: the FeatureMutator interface
  in internal/generic uses an unexported beginFeature() method, so the
  clusterrole Mutator could never satisfy it across package boundaries.
  Replaced with flat intent lists and fixed category ordering (metadata →
  rules → aggregation), which matches the actual runtime behavior.

- Update mutator test: replace TestMutator_MultipleFeatures (which called
  unexported beginFeature() directly) with TestMutator_MultipleMutations
  that exercises the public API only.

- Update docs to reflect that namespace must be empty (not just optional)
  for cluster-scoped resources, matching ValidateBase() behavior.

- Update mutation ordering docs to accurately describe flat category
  ordering rather than per-feature boundaries.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Defensively allocate a local zero-value slice when a nil pointer is
passed, matching the pattern used by NewConfigMapDataEditor. This
prevents nil-pointer panics in Raw()/AddRule()/RemoveRuleByIndex()/Clear().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…itation

- Remove redundant "valid clusterrole without namespace" test case that
  was identical to "valid clusterrole"
- Document cluster-scoped ownership limitation in clusterrole docs
- Add comment in example about namespaced owner limitation with real clusters

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ExternalRules

- Adopt per-feature planning pattern (beginFeature/featurePlan) in the
  ClusterRole mutator for consistency with ConfigMap and Deployment
  primitives, ensuring mutation ordering matches registration order
- Update ownership limitation docs to reflect actual framework behavior:
  scope mismatch is detected and owner ref is skipped with a log message,
  rather than rejecting the reconcile
- Remove PreserveExternalRules from example and docs feature-gating
  walkthrough to avoid conflict where disabled feature-gated rules would
  be preserved from the live object
- Add documentation note about PreserveExternalRules trade-off with
  feature-gated mutations
- Remove duplicate cluster-scoped validation tests in builder_static_test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pplicator

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Initialize plans slice and active pointer directly in the constructor
to avoid creating an empty feature when mutate_helper.go calls
beginFeature() on the first feature.

Mirrors the fix applied to deployment and configmap mutators in #42.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use set-based (order-independent) comparison for PolicyRule slices in
  PreserveExternalRules to avoid duplicating rules after webhook normalization
- DeepCopy AggregationRule in SetAggregationRule to prevent caller mutations
  from affecting recorded intent
- Remove duplicate nil/name validation in Build(), delegating to
  generic.StaticBuilder.ValidateBase() consistent with ConfigMap primitive
- Add tests for reordered-but-equivalent rules and aggregation rule immutability

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After rebasing on main, the clusterrole mutator still had the unexported
beginFeature() method. Rename to BeginFeature() to align with the
exported FeatureMutator interface in internal/generic/resource_workload.go.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update comments and documentation for PreserveExternalRules to accurately
describe that slice fields are compared as unordered sets. Fix GoDoc
capitalization of ClusterRole in Mutation type comment.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Delegate state and With* calls directly to the generic builder instead
of storing mutations/flavors/extractors locally and replaying them in
Build(). This matches the pattern used by other primitives (configmap,
deployment) and avoids duplicated plumbing that could drift.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ate docs

- CoreRulesMutation and VersionLabelMutation now use nil Feature (truly
  unconditional) instead of a no-op NewResourceFeature(version, nil)
- Add ClusterRole to the Built-in Primitives table in primitives.md
- Add PolicyRulesEditor to the Mutation Editors table in primitives.md
- Mention PolicyRulesEditor in clusterrole.md capabilities table

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…false ordering claims in tests

- Update docs/primitives.md to reflect that PolicyRulesEditor applies to
  both Role and ClusterRole, not just ClusterRole
- Rename and re-scope two tests that falsely claimed to verify operation
  ordering when the assertions were order-independent

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ource-level tests

- TestBuilder_WithMutation now uses a Mutation with a non-nil Mutate func
  so the test models valid usage instead of an impossible configuration.
- Add comprehensive resource-level tests for ClusterRole: Identity(),
  Object() deep-copy, Mutate with default and custom field applicators,
  mutation execution, feature ordering, and data extraction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ot set semantics

The docs claimed slice fields were compared as "unordered sets" but the
implementation treats duplicates as significant (multiset equality).
Updated comments to accurately describe the behavior rather than changing
the comparison logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align with configmap and deployment mutators — NewMutator no longer
creates an initial feature plan. BeginFeature must be called before
registering any mutations. Updates all tests accordingly and adds
constructor/plan invariant tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tive

Align with the framework's move to Server-Side Apply. Remove
DefaultFieldApplicator, WithCustomFieldApplicator,
WithFieldApplicationFlavor, the flavors.go file, and all associated
tests. Update builder to use the new generic constructor signature
(without defaultApplicator). Update Mutate tests to use Object() output
instead of empty structs. Strip field applicator and flavor sections
from primitive docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Document why clusterrole-primitive example is excluded from make run-examples
- Use slices.Delete in PolicyRulesEditor.RemoveRuleByIndex to avoid memory retention

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… discarding edits

Passing a nil *[]PolicyRule pointer to NewPolicyRulesEditor is a programmer
error — edits would be recorded against a local variable and never propagate
back to the Kubernetes object. Fail fast with a clear panic message instead
of silently swallowing the mistake.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mutator methods that access m.active (EditObjectMetadata, EditRules,
SetAggregationRule) now call requireActive() which panics with a clear
message instead of an opaque nil-pointer dereference.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants